fix(flows): save-safety — no silent live-arming on update, flag empty runs, close resume_flow_run HITL bypass#4889
Conversation
…pty-flow runs flows_update had no equivalent of flows_create's B29 Rule 1: saving a graph with an automatic trigger (schedule/app_event/webhook) onto an already enabled flow whose previous trigger was manual/none would arm it live with no user review (confirmed live: a flow started firing on an unreviewed 8am schedule). flows_update now force-disables the flow in that manual->automatic transition case only, threaded into the same guarded store write to avoid a TOCTOU window. Also: flows_run on a graph with no actionable nodes (trigger only, or no edges wiring up any downstream node) reported a plain "completed" with no indication nothing happened. It now logs a warning and attaches a human-readable "note" to the run result.
flows_build runs workflow_builder under AgentTurnOrigin::Cli, which makes the ApprovalGate auto-allow every external_effect tool. Issue tinyhumansai#4881 added resume_flow_run (external_effect=true, "ADVANCES A REAL RUN") and cancel_flow_run to the workflow_builder belt without extending FLOWS_BUILD_HIDDEN_TOOLS, reopening the tinyhumansai#4593 HITL hole: an authoring turn could approve a live run's parked Slack/Gmail/HTTP node with zero human confirmation. Add both to the hide-list alongside run_flow/run_workflow. create_workflow/duplicate_flow stay visible — both are forced born-disabled, so an unattended call can't leave anything live.
📝 WalkthroughWalkthroughFlow execution now reports trigger-only runs, flow edits can atomically disable newly automatic enabled flows, and the direct builder path hides additional live-run controls. ChangesFlow safety and execution behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant flows_update
participant update_flow_graph
participant flow_definitions
Client->>flows_update: submit trigger-changing flow update
flows_update->>flows_update: detect enabled manual-to-automatic transition
flows_update->>update_flow_graph: pass enabled_override=false
update_flow_graph->>flow_definitions: guarded update of graph and enabled state
flow_definitions-->>Client: return updated disabled flow and logs
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eef793c31
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let should_disarm = now_auto && existing.enabled && !was_auto; | ||
| let enabled_override = should_disarm.then_some(false); |
There was a problem hiding this comment.
When flows_update starts from a disabled manual flow and another request enables it before update_flow_graph rereads the row (the canvas and save_workflow both call this without expected_version), should_disarm stays false from the stale outer read. The store then preserves the newly true enabled value while saving the automatic trigger and the later rebind path arms it, leaving the same silent live-arming race this change is meant to close. The disarm decision needs to be made against the current row used by the guarded update, or force enabled=false for manual/none → automatic transitions regardless of the stale existing.enabled.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/flows/store_tests.rs (1)
94-102: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd direct tests for both
enabled_overridebranches.Both calls pass
None, so these tests do not verify thatSome(false)persists the override or thatNoneleaves the database column untouched. Add store-level regression coverage for both contracts.As per coding guidelines, “New or changed behavior must include tests.”
Also applies to: 201-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/store_tests.rs` around lines 94 - 102, Add store-level regression tests around update_flow_graph covering both enabled_override contracts: verify Some(false) persists the disabled override, and verify None leaves the existing database column unchanged. Keep the current None-based update coverage, and use distinct setup/assertions for each branch.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 269-302: Add a test alongside
flows_run_on_graph_with_actionable_nodes_has_no_empty_flow_note with a trigger
and a separate downstream component whose non-trigger nodes are edge-connected
but unreachable from the trigger. Run the graph through flows_create and
flows_run, then assert the outcome includes the existing “nothing to execute”
note, covering unreachable actionable components.
- Around line 646-698: Add a deterministic concurrency regression test alongside
flows_update_disables_on_manual_to_automatic_trigger_transition_when_enabled:
pause flows_update after its initial flow read, enable the disabled manual flow
through the normal update path, then resume the pending update with an automatic
trigger graph. Assert the automatic graph is persisted disabled, the
response/logs reflect auto-disabling, and no schedule job is bound; coordinate
the interleaving with synchronization primitives rather than timing sleeps.
In `@src/openhuman/flows/ops.rs`:
- Around line 378-384: The graph_has_actionable_nodes function must traverse
from the trigger and return true only when a non-trigger node is reachable,
rather than merely counting non-trigger nodes and any edges. In
src/openhuman/flows/ops.rs:378-384, update this reachability check; in
src/openhuman/flows/ops_tests.rs:269-302, add a disconnected component
containing edges and assert that the empty-flow note still appears.
---
Outside diff comments:
In `@src/openhuman/flows/store_tests.rs`:
- Around line 94-102: Add store-level regression tests around update_flow_graph
covering both enabled_override contracts: verify Some(false) persists the
disabled override, and verify None leaves the existing database column
unchanged. Keep the current None-based update coverage, and use distinct
setup/assertions for each branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36c9638b-334b-43b1-b709-6a55f7fba9ee
📒 Files selected for processing (4)
src/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/flows/store.rssrc/openhuman/flows/store_tests.rs
| /// A graph with a real downstream node, wired up by an edge, must NOT carry | ||
| /// the "nothing to run" note — only a graph with no actionable nodes at all. | ||
| /// Uses `output_parser` nodes (like the approval-gated fixture above) rather | ||
| /// than an `agent`/`tool_call` node so the run completes deterministically | ||
| /// without needing a configured LLM provider or network access. | ||
| #[tokio::test] | ||
| async fn flows_run_on_graph_with_actionable_nodes_has_no_empty_flow_note() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let config = test_config(&tmp); | ||
|
|
||
| let graph = json!({ | ||
| "name": "has-work", | ||
| "nodes": [ | ||
| { "id": "t", "kind": "trigger", "name": "Trigger" }, | ||
| { "id": "downstream", "kind": "output_parser", "name": "Downstream" } | ||
| ], | ||
| "edges": [ | ||
| { "from_node": "t", "to_node": "downstream" } | ||
| ] | ||
| }); | ||
| let created = flows_create(&config, "has-work".to_string(), graph, false) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let outcome = flows_run(&config, &created.value.id, json!({}), FlowRunTrigger::Rpc) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!( | ||
| outcome.value.get("note").is_none(), | ||
| "a graph with real downstream nodes must not get the empty-flow note, got: {:?}", | ||
| outcome.value.get("note") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover an edged but unreachable downstream component.
Add a fixture where non-trigger nodes have edges among themselves but no path from the trigger. It should still receive the “nothing to execute” note; the current implementation would miss it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/flows/ops_tests.rs` around lines 269 - 302, Add a test
alongside flows_run_on_graph_with_actionable_nodes_has_no_empty_flow_note with a
trigger and a separate downstream component whose non-trigger nodes are
edge-connected but unreachable from the trigger. Run the graph through
flows_create and flows_run, then assert the outcome includes the existing
“nothing to execute” note, covering unreachable actionable components.
Source: Coding guidelines
| #[tokio::test] | ||
| async fn flows_update_disables_on_manual_to_automatic_trigger_transition_when_enabled() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let config = test_config(&tmp); | ||
|
|
||
| // A manual-trigger flow persists enabled straight from create (Rule 1 | ||
| // only gates automatic triggers). | ||
| let created = flows_create( | ||
| &config, | ||
| "manual-then-scheduled".to_string(), | ||
| manual_trigger_graph(), | ||
| false, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| assert!(created.value.enabled, "manual-trigger flows create enabled"); | ||
|
|
||
| // Saving an automatic-trigger graph onto that enabled flow must disarm | ||
| // it — not go live unattended. | ||
| let updated = flows_update( | ||
| &config, | ||
| &created.value.id, | ||
| None, | ||
| Some(schedule_trigger_graph("0 8 * * *")), | ||
| None, | ||
| None, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!( | ||
| !updated.value.enabled, | ||
| "an enabled flow whose trigger just changed from manual to automatic must be \ | ||
| auto-disabled, not armed live" | ||
| ); | ||
| assert!( | ||
| updated.logs.iter().any(|l| l.contains("auto-disabled")), | ||
| "the disarm must be surfaced in the outcome logs, got: {:?}", | ||
| updated.logs | ||
| ); | ||
|
|
||
| // Persisted, not just returned in-memory. | ||
| let reloaded = flows_get(&config, &created.value.id).await.unwrap(); | ||
| assert!(!reloaded.value.enabled); | ||
|
|
||
| // And no cron job was left bound — the flow never actually went live. | ||
| assert!( | ||
| crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id) | ||
| .unwrap() | ||
| .is_none(), | ||
| "an auto-disabled flow must not have its schedule cron job bound" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exercise the concurrent-enable transition.
This sequential test passes despite the stale-snapshot race in flows_update. Add a deterministic regression where a disabled manual flow is enabled between the initial read and guarded graph write; the automatic graph must never persist as enabled.
As per coding guidelines, “New or changed behavior must include tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/flows/ops_tests.rs` around lines 646 - 698, Add a deterministic
concurrency regression test alongside
flows_update_disables_on_manual_to_automatic_trigger_transition_when_enabled:
pause flows_update after its initial flow read, enable the disabled manual flow
through the normal update path, then resume the pending update with an automatic
trigger graph. Assert the automatic graph is persisted disabled, the
response/logs reflect auto-disabling, and no schedule job is bound; coordinate
the interleaving with synchronization primitives rather than timing sleeps.
Source: Coding guidelines
| pub(crate) fn graph_has_actionable_nodes(graph: &WorkflowGraph) -> bool { | ||
| let non_trigger_nodes = graph | ||
| .nodes | ||
| .iter() | ||
| .filter(|n| n.kind != NodeKind::Trigger) | ||
| .count(); | ||
| non_trigger_nodes > 0 && !graph.edges.is_empty() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file map ==\n'
git ls-files 'src/openhuman/flows/*' | sed -n '1,120p'
printf '\n== outline ops.rs ==\n'
ast-grep outline src/openhuman/flows/ops.rs --view expanded || true
printf '\n== outline ops_tests.rs ==\n'
ast-grep outline src/openhuman/flows/ops_tests.rs --view expanded || true
printf '\n== relevant snippets ==\n'
sed -n '340,430p' src/openhuman/flows/ops.rs
printf '\n--- tests ---\n'
sed -n '240,340p' src/openhuman/flows/ops_tests.rsRepository: tinyhumansai/openhuman
Length of output: 28321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== usages of graph_has_actionable_nodes ==\n'
rg -n "graph_has_actionable_nodes" src/openhuman/flows -S
printf '\n== flows_run note handling ==\n'
sed -n '2990,3095p' src/openhuman/flows/ops.rs
printf '\n== graph validation for connectivity / reachability ==\n'
rg -n "connected|reachable|edge.*trigger|trigger.*edge|disconnected" src/openhuman/flows/ops.rs src/openhuman/flows/*.rs -SRepository: tinyhumansai/openhuman
Length of output: 18674
Walk the graph from the trigger before suppressing the empty-flow note
non_trigger_nodes > 0 && !graph.edges.is_empty() still treats disconnected edges as actionable. Require at least one reachable non-trigger node instead.
src/openhuman/flows/ops_tests.rs: add a disconnected component with edges and assert the empty-flow note still appears.
📍 Affects 2 files
src/openhuman/flows/ops.rs#L378-L384(this comment)src/openhuman/flows/ops_tests.rs#L269-L302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/flows/ops.rs` around lines 378 - 384, The
graph_has_actionable_nodes function must traverse from the trigger and return
true only when a non-trigger node is reachable, rather than merely counting
non-trigger nodes and any edges. In src/openhuman/flows/ops.rs:378-384, update
this reachability check; in src/openhuman/flows/ops_tests.rs:269-302, add a
disconnected component containing edges and assert that the empty-flow note
still appears.
1 participant
Summary
Three safety fixes in the shared
flowsdomain (all main-compatible, no #4881 dependency), surfaced by live E2E testing + a bug-sweep of the workflow-builder subsystem.Problems & fixes
1.
flows_updatecould silently arm a live auto-trigger.flows_createforces auto-trigger graphs born-disabled (B29 Rule 1), butflows_updatepreservedenabled=true— so editing a manual/enabled flow's graph to aschedule/webhook/app_eventtrigger armed it live, unreviewed (observed: a flow went live on an 8am schedule the user never saw). Fix:flows_updatenow disarms (enabled=false) whennow_auto && existing.enabled && !was_auto(manual→automatic transition on an enabled flow), threaded into the same optimistic-concurrency-guarded UPDATE (no TOCTOU). Reuses the existingtrigger_is_automaticpredicate.2. Running an empty/trigger-only flow reported "completed" doing nothing. Fix:
graph_has_actionable_nodesguard inflows_run→warn!+ anoteon the result ("nothing to execute"); status shape unchanged (non-breaking).3. CRITICAL —
resume_flow_runHITL bypass.FLOWS_BUILD_HIDDEN_TOOLSonly hidrun_workflow/run_flow, but the widened builder belt includesresume_flow_run(external_effect=true). Underflows_build's Cli-origin auto-allow, a copilot authoring turn could resume a live run and fire its approved Slack/Gmail/HTTP nodes with zero human confirmation — reopening the hole PR #4593 closed. Fix: hideresume_flow_run+cancel_flow_runfrom the builder belt.create_workflow/duplicate_flowleft visible (forced born-disabled; commented).Testing
cargo test --lib openhuman::flows::→ 365 passed, 0 failed.resume_flow_run/cancel_flow_runare hidden whilecreate_workflow/duplicate_flowstay visible.GGML_NATIVE=OFF cargo check+cargo fmt --checkclean.Acceptance criteria
Summary by CodeRabbit
Bug Fixes
Safety Improvements